static addr loopin: add support for sweepless sweep batching #844

sputn1ck · 2024-11-04T20:10:39Z

This PR adds support for serverside sweepless sweep batching, by listening for notifications from the server and responding
with the signature.

Only the 8 last commits are for review

starius

LGTM! It's very cool!

Added several comments.

starius · 2024-11-13T13:48:05Z

looprpc/client.proto

@@ -1617,7 +1617,7 @@ message ListStaticAddressDepositsRequest {
    /*
    Filters the list of all stored deposits by deposit state.
    */
-    DepositState state_filter = 1;
+    string state_filter = 1;


Why is this needed? Now it is not clear from the schema what are the possible values here. Also it is easy to confuse DepositState and StaticAddressLoopInSwapState.

starius · 2024-11-13T13:50:39Z

swapserverrpc/staticaddr.proto

-message FetchSweeplessSweepTxRequest {
-    // The swap hash of the swap that the client wants to fetch the sweepless
-    // sweep tx for.
-    bytes swap_hash = 1;
-}
-
-message FetchSweeplessSweepTxResponse {
-    // The address that the server wants to sweep the static address deposits
-    // to.
-    string sweep_addr = 1;
-
-    // A map of fee rate to the nonces.
-    map<uint64, ServerSweeplessSigningInfo> fee_rate_to_nonces = 2;
-}
-
-message ServerSweeplessSigningInfo {
-    // The nonces that the server used to generate the partial sweepless tx
-    // sigs.
-    repeated bytes nonces = 1;
-}


I think, this should be removed later, in commit "staticaddr/loopin: remove unused code".

Not sure, as this commit changes the functionality of what we do

starius · 2024-11-14T04:27:07Z

staticaddr/loopin/interface.go

+
+	// FetchLoopInByHash returns the loop-in swap with the given hash.
+	FetchLoopInByHash(ctx context.Context, swapHash lntypes.Hash) (
+		*StaticAddressLoopIn, error)


This commit does not build:

# github.com/lightninglabs/loop/staticaddr/loopin staticaddr/loopin/actions.go:791:23: undefined: looprpc.FetchSweeplessSweepTxRequest

I think it should include SQL changes.

+1, the failure starts with the previous commit

as I'm changing the proto messages, should I add the changes in fsm/actions etc. to the proto commit?

starius · 2024-11-14T04:33:47Z

staticaddr/loopin/manager.go

 		case <-ctx.Done():
 			return ctx.Err()
 		}
 	}
 }

+// notifyNotFinished notifies the server that a swap is not finished by
+// sending an empty signature map.


"by sending an empty signature map"
This is a part of API.

I propose to add this to description of PushStaticAddressSweeplessSigsRequest.signing_info.

Also, we can make this more explicit by making a oneof: a map or an error (string). If the swap is not finished or any other error happened during signing, we would send PushStaticAddressSweeplessSigsRequest with an error message instead of a map.

starius · 2024-11-14T04:36:44Z

staticaddr/loopin/manager.go

+
+	// If the loop-in is not in the Succeeded state we return an
+	// error.
+	if loopIn.state != Succeeded {


~~Should it also accept FetchSignPushSweeplessSweepTx and SucceededSweeplessSigFailed states?~~

They are removed in a subsequent commit. Maybe it worth adding them here and removing in commit "staticaddr/loopin: update fsm for server ntfn sigs".

starius · 2024-11-14T04:48:28Z

staticaddr/loopin/manager.go

+
+	// We'll now sign for every deposit that is part of the loop-in.
+	responseMap := make(map[string]*looprpc.ClientSweeplessSigningInfo)
+	for depositOutpoint, nonce := range req.DepositToNonces {


I propose to factor out the body of this for to a function.

starius · 2024-11-14T04:49:06Z

staticaddr/loopin/manager.go

+	// Check if all the deposits requested are part of the loop-in and
+	// find them in the requested sweep.
+	depositToIdxMap := make(map[string]int)
+	for reqOutpoint := range req.DepositToNonces {


I propose to factor out the body of this for to a function.

starius · 2024-11-14T04:49:56Z

staticaddr/loopin/manager.go

+
+		for i, txIn := range sweepTx.TxIn {
+			if txIn.PreviousOutPoint.String() == reqOutpoint {
+				depositToIdxMap[reqOutpoint] = i


Let's also check that req.DepositToNonces doesn't include duplicate deposits. We can add a check that depositToIdxMap[reqOutpoint] does not pre-exist here.

staticaddr/loopin/manager.go

starius · 2024-11-14T04:53:54Z

staticaddr/loopin/manager.go

+	_, err = m.cfg.Server.PushStaticAddressSweeplessSigs(
+		ctx, &looprpc.PushStaticAddressSweeplessSigsRequest{
+			SwapHash:    loopIn.SwapHash[:],
+			Txid:        txHash[:],


IIUC, server knows txhash already, because it depends on unsigned parts only. Can we remove the field?

I'd rather keep it, as the txhash is a unique identifier for the explicit sweep right?

hieblmi

This is great work @sputn1ck, the client notifications are super nice and will surely be further extended.
I left some nits and potential simplifications. Will test it now.

swapserverrpc/server.proto

hieblmi · 2024-11-14T13:40:16Z

cmd/loop/staticaddr.go

@@ -287,48 +287,9 @@ func listDeposits(ctx *cli.Context) error {
 	}
 	defer cleanup()

-	var filterState looprpc.DepositState


this commit to be reverted.

swapserverrpc/staticaddr.proto

notifications/manager.go

staticaddr/loopin/manager.go

hieblmi · 2024-11-14T14:07:19Z

staticaddr/loopin/manager.go

+	// Check if all the deposits requested are part of the loop-in and
+	// find them in the requested sweep.
+	depositToIdxMap := make(map[string]int)
+	for reqOutpoint := range req.DepositToNonces {


hieblmi · 2024-11-14T14:17:31Z

staticaddr/loopin/manager.go

+			Index: prevout.OutputIndex,
+		}] = &wire.TxOut{
+			Value:    int64(req.PrevoutInfo[i].Value),
+			PkScript: req.PrevoutInfo[i].PkScript,


Isn't the pkscript the same for all deposits. If so couldn't we just call

loopIn.toPrevOuts(loopIn.Deposits, req.PrevoutInfo[0].PkScript)

staticaddr/loopin/manager.go

hieblmi

tAck thanks for the batching preparations!

We should fix the mentioned clean-up of musig sessions, they get auto-cleaned if the signing runs smoothly, and then cleaning fails with the mentioned error. We should only cancel sessions if the signing doesn't take place.

We should also fix the minor issues from the previous review.

staticaddr/loopin/manager.go

hieblmi · 2024-11-15T13:35:05Z

staticaddr/loopin/interface.go

+
+	// FetchLoopInByHash returns the loop-in swap with the given hash.
+	FetchLoopInByHash(ctx context.Context, swapHash lntypes.Hash) (
+		*StaticAddressLoopIn, error)


+1, the failure starts with the previous commit

This commit adds a new notification to the proto, which is used by the server to request signatures for a deposit of a static loop in.

This commit adds support for the new notification of type NotificationTypeStaticLoopInSweepRequest. This notification is sent when a sweep request is received from the server.

This commit adds the listening for sweep requests from the server. On a sweep request the loopin manager will fetch the loop in from the db, do sanity and safety checks and then sign the psbt for the input and send it back to the server.

This commit updates the loop in FSM to handle the new server based

sputn1ck marked this pull request as draft November 4, 2024 20:23

sputn1ck force-pushed the static_sweep_batching branch from 1f5c270 to 92f540b Compare November 4, 2024 20:57

hieblmi force-pushed the static-addr-staging branch 3 times, most recently from 60ff75b to ec3070f Compare November 5, 2024 18:22

levmi assigned sputn1ck Nov 7, 2024

sputn1ck force-pushed the static_sweep_batching branch 4 times, most recently from bbff3d0 to 2cbd456 Compare November 8, 2024 09:10

sputn1ck requested review from bhandras, hieblmi and starius November 11, 2024 10:59

sputn1ck marked this pull request as ready for review November 12, 2024 16:17

starius approved these changes Nov 14, 2024

View reviewed changes

sputn1ck mentioned this pull request Nov 14, 2024

static loop in: More fee versions for sweepless sweep #845

Closed

sputn1ck force-pushed the static_sweep_batching branch from 2cbd456 to 985d154 Compare November 14, 2024 14:36

hieblmi reviewed Nov 14, 2024

View reviewed changes

hieblmi force-pushed the static-addr-staging branch from 44092a7 to e54ccfd Compare November 14, 2024 15:31

sputn1ck force-pushed the static_sweep_batching branch from 985d154 to 86c65c0 Compare November 14, 2024 15:34

hieblmi self-requested a review November 15, 2024 13:32

hieblmi approved these changes Nov 15, 2024

View reviewed changes

sputn1ck force-pushed the static_sweep_batching branch 2 times, most recently from e3ec4e5 to 7d50c70 Compare November 18, 2024 13:30

hieblmi force-pushed the static-addr-staging branch from e54ccfd to 0d87b69 Compare November 18, 2024 14:58

sputn1ck added 5 commits November 18, 2024 17:11

swapserverrpc: add new functions

893f30c

This commit adds a new notification to the proto, which is used by the server to request signatures for a deposit of a static loop in.

notifications: add support for new ntfn

7e927d1

This commit adds support for the new notification of type NotificationTypeStaticLoopInSweepRequest. This notification is sent when a sweep request is received from the server.

staticaddr/sql_store: Add fetching loop in by hash

3a06bec

staticaddr/loopin: update fsm for server ntfn sigs

dea6c0e

This commit updates the loop in FSM to handle the new server based

sputn1ck added 2 commits November 18, 2024 17:11

loopd/staticaddr: add ntfn manager

a3ebc6d

staticaddr/loopin: remove unused code

144b62c

sputn1ck force-pushed the static_sweep_batching branch from 7d50c70 to 144b62c Compare November 18, 2024 16:12

sputn1ck merged commit 627a830 into lightninglabs:static-addr-staging Nov 18, 2024
4 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

static addr loopin: add support for sweepless sweep batching #844

static addr loopin: add support for sweepless sweep batching #844

sputn1ck commented Nov 4, 2024 •

edited

Loading

starius left a comment

starius Nov 13, 2024

starius Nov 13, 2024

sputn1ck Nov 14, 2024

starius Nov 14, 2024

hieblmi Nov 15, 2024

sputn1ck Nov 18, 2024

starius Nov 14, 2024

starius Nov 14, 2024

starius Nov 14, 2024

starius Nov 14, 2024

hieblmi Nov 14, 2024

starius Nov 14, 2024

starius Nov 14, 2024

sputn1ck Nov 14, 2024

hieblmi left a comment •

edited

Loading

hieblmi Nov 14, 2024

hieblmi Nov 14, 2024

hieblmi Nov 14, 2024

hieblmi left a comment •

edited

Loading

hieblmi Nov 15, 2024

static addr loopin: add support for sweepless sweep batching #844

static addr loopin: add support for sweepless sweep batching #844

Conversation

sputn1ck commented Nov 4, 2024 • edited Loading

starius left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hieblmi left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hieblmi left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sputn1ck commented Nov 4, 2024 •

edited

Loading

hieblmi left a comment •

edited

Loading

hieblmi left a comment •

edited

Loading